-
Notifications
You must be signed in to change notification settings - Fork 504
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixes incorrect filepath reporting in sarif output & added e2e tests for sarif output #863
Conversation
Codecov Report
@@ Coverage Diff @@
## master #863 +/- ##
==========================================
- Coverage 78.59% 78.41% -0.19%
==========================================
Files 167 168 +1
Lines 4452 4470 +18
==========================================
+ Hits 3499 3505 +6
- Misses 732 741 +9
- Partials 221 224 +3
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please run go mod tidy
on this PR as well
pkg/utils/dir.go
Outdated
@@ -30,7 +30,7 @@ func GenerateTempDir() string { | |||
func IsDirExists(dir string) bool { | |||
_, err := os.Stat(dir) | |||
if os.IsNotExist(err) { | |||
zap.S().Debug("Directory %s does not exist.", dir) | |||
zap.S().Errorf("Directory %s does not exist.", dir) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
error string should not start with a capital alphabet
zap.S().Errorf("file %s does not exist.", path) | ||
} else { | ||
zap.S().Errorf("unable to fetch file info for path %s.", path) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method may panic. os.Stat() returns nil file info on an error and we are just logging the error and not returning.
@@ -0,0 +1,19 @@ | |||
package utils |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing copyright header?
@@ -73,3 +73,23 @@ func AreEqualJSON(s1, s2 string) (bool, error) { | |||
|
|||
return reflect.DeepEqual(o1, o2), nil | |||
} | |||
|
|||
// AreEqualJSONBytes validate if two json byte arrays are equal | |||
func AreEqualJSONBytes(b1, b2 []byte) (bool, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to use AreEqualJSON
already present in the file?
If not, I think we can refactor AreEqualJSON
func.
pkg/writer/sarif.go
Outdated
|
||
func getAbsoluteFilePath(resourcePath, filePath string) string { | ||
if !filepath.IsAbs(resourcePath) { | ||
resourcePath, _ = filepath.Abs(resourcePath) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even though error seems unlikely here, I think we should handle error case too !!
} | ||
|
||
// SarifTemplateAWSAMIViolation string | ||
const SarifTemplateAWSAMIViolation = `{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wondering if we can use golden text files instead of a template. Is it possible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think It'll unnecessary add another step of file processing. Maybe we can take this point up in some other refactor phase as such templates are being used in other places as well.
test/helper/helper.go
Outdated
// GetAbsoluteFilePathForSarif helper for sarif path | ||
func GetAbsoluteFilePathForSarif(resourcePath, filePath string) string { | ||
if !filepath.IsAbs(resourcePath) { | ||
resourcePath, _ = filepath.Abs(resourcePath) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should handle the error case here too.
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
fixes #861